Stop freeing child scene tiles when the TileMap is removed from the tree #92189
Conversation
| @@ -1195,15 +1195,15 @@ void TileMapLayer::_navigation_draw_cell_debug(const RID &p_canvas_item, const V | |||
|
|
|||
| void TileMapLayer::_scenes_update(bool p_force_cleanup) { | |||
| // Check if we should cleanup everything. | |||
| bool forced_cleanup = p_force_cleanup || !enabled || !is_inside_tree() || tile_set.is_null(); | |||
There was a problem hiding this comment.
Disabling a layer should still remove the nodes I think.
There was a problem hiding this comment.
Hm, should disabling a layer remove the nodes? Or just disable them?
I'm imagining a scenario where you disable/re-enable a layer. You'd probably want your scene nodes to not get freed/re-instantiated.
There was a problem hiding this comment.
Disabling a layer makes it invisible and non-interactable, so the same should apply to nodes. If you want to achieve that while keeping the nodes' state, the solution could be temporarily removing them from scene tree and re-adding once the layer is enabled. It sounds complex to implement though.
There was a problem hiding this comment.
Hm, not only that, but it also seems like one should still be able to find/access/modify the nodes in the Layer subtree even if the Layer is disabled.
This is a more impactful change, but one solution might be:
- Parent tile scene nodes to the Layer instead of the TileMap
- Now, I think setting Node.ProcessMode and CanvasItem.Visible on the Layer cover the exact functionality that Layer.enabled does
There was a problem hiding this comment.
Maybe we could go with removing for now, just to fix the regression, and improve it in a follow-up. Changing behavior of layer disabling wouldn't count as bugfix.
Gerfalerf
force-pushed
the
tilemap-child-scenes-deletion
branch
from
2439c57
to
dc5ed06
Compare
Gerfalerf
force-pushed
the
tilemap-child-scenes-deletion
branch
from
dc5ed06
to
0f26c84
Compare
| @@ -1691,8 +1691,8 @@ void TileMapLayer::_notification(int p_what) { | |||
|
|
|||
| case NOTIFICATION_EXIT_TREE: { | |||
| dirty.flags[DIRTY_FLAGS_LAYER_IN_TREE] = true; | |||
| // Update immediately on exiting, and force cleanup. | |||
| _internal_update(true); | |||
| // Update immediately on exiting, but do not force cleanup. | |||
There was a problem hiding this comment.
Why this change? The force part is necessary, please explain the change as it affects far more than just the scenes, because I wrote this code and the forced update is required, so please detail how you established that this is a necessary change and that it won't cause issues
I explicitly added the forced part because things didn't work correctly without it (otherwise I'd not have gone through all that work to add it everywhere):
This change reintroduces:
There was a problem hiding this comment.
I took a look at your repro with the light occluder, and it looks like while force cleaning the atlas tiles did solve it, scene tiles never had that issue, and should not have been changed to force clean when the TileMap is removed from the tree.
Force cleaning the scene tile nodes when the TileMap is removed from the tree erases any state those nodes have accumulated since being instantiated and causes them to be reloaded in their default state when the TileMap is eventually re-added to the tree.
For a solution that solves both issues, I wish I could just take the force_cleanup out of _scenes_update, but that causes other problems. Now that the scene tile nodes haven't been wiped out, the rest of _scenes_update tries to access them in a bad non-deferred way (which is part of why I originally wanted to switch to use _queue_internal_update).
To do this right, I think perhaps I will either have to split up the atlas/scene tile update code paths a bit more, or split up the force_clean/update code paths a bit more.
There was a problem hiding this comment.
It did so before so it wasn't changed by this, it was restored to how it was done before the thread fixes, my PR restored the behavior before that that had been done for a long time, so this change shouldn't be made like this, please restore it and we can work with a solution down the line, but this change is invalid
Again, scene tiles were deleted when exiting the tree for a long time, but that behavior was accidentally removed for a short while after thread fixes were applied, the reason for the forced cleanup is that is_inside_tree returns false when exiting the tree, so it worked because the deferred call. But the deferred call was thread unsafe
The problem is that there'd be no way to solve this without failing to remove the scene tiles even when cleaning up at destruction, since this was the behavior previously I'd say it's invalid to change it this way, unless it's safe to not force cleaning up when deleting the node itself
But it should be done without touching the forced cleanup of other parts
There was a problem hiding this comment.
The fact that the updates break things if the force is delayed implies this is a more involved problem and that the clearing of the tiles is assumed and built into the process IMO, I'm not convinced it's a bug, but a possible limitation that might need more workarounds and other solutions, and at least might not be appropriate for the 4.3 release given that it risks causing bugs and issues
For the forced part a simple solution would be to add two arguments, or to restore the old variable tracking if we're in the destructor and free them only then, but I'm not sure it's trivial to implement this change without a lot of other changes, all making this fragile
But I think we should wait for the opinion of groud on this
But I feel that using scene tiles should be adjusted to adapt to this, by having them handle states in a way that handles this, rather than keeping them around, it makes a lot of processing with the tiles more manageable I think
We could also implement some way to track and retain state, but I don't know that scene tiles being stateful is something that's guaranteed to be supported, we don't allow providing per-tile options to them for example, there's a proposal for that I believe
There was a problem hiding this comment.
Definitely, I recognize the change is invalid as-is, and I'm not proposing that we submit it yet. I super don't want to re-introduce the issue that you linked.
It sounds like you're saying that you think maybe scene tile nodes should be deleted and re-instantiated when the TileMap exits/re-enters the tree. If that is true, then you're right that this is not a bug, and we can abandon trying to fix this PR.
However, that seems at odds with Godot's tree model to delete and re-instantiate those nodes when their parent is simply added/removed. Happy to wait for others' opinions on this as well.
If we aren't going to fix this issue, I think this behavior should be documented here, and that the child nodes that are created should maybe be changed to be "Internal" children of the TileMap.
There was a problem hiding this comment.
However, that seems at odds with Godot's tree model to delete and re-instantiate those nodes when their parent is simply added/removed. Happy to wait for others' opinions on this as well.
That's arguably different, these aren't normal node children but internally generated nodes, handled by the node itself.
But let's see what the maintainers for this part says on the desired behavior, or the limitations we should follow. The solution should handle things safely so must be adjusted to the change here, so depends on the assumptions that was made for the rest of the code, which seems to be that the scenes should be removed on exiting the tree, and the question is if that's desired or not, but the behavior is not fully accidental, it might be a case of copy-pasting some of the code, but the condition for deletion is explicit.
Can you point to a time in the history of this node where it didn't delete the tiles when exiting the tree? It did after #67330
|
Thanks for contributing! Soooo, to be honest I am not sure I really want to change the behavior of this, and I think the original "bug" might not really be one. While I understand the rationale of not wanting the child nodes to be freed, scene as tiles are not really like normal scenes. Those children are internal nodes, managed by the TileMapLayer node. And keeping runtime internal data allocated when the TileMapLayer node is not in use makes the implementation more complex IMO. The same rationale could be applied to CanvasItems, physics shapes and other sub resources too, which could be disabled instead of removed, but I don't want to go down that rabbit hole without it being absolutely necessary. I think this is really a limitation of the TileMapLayer node anyway, the same way you cannot, like, set properties of scene tiles in the map, you kind of have to deal with the TileMapLayer node managing the scenes for you. The original use case for scenes as tiles was more about, like, having animated / super simple objects, but not much more. I'd say most of the properties you would want to keep persistent should be stored in a resource or in the TileMapLayer node instead. In general, don't really want to make the "scene as tiles" subsystem more complex, because the more complex it gets, the closer it becomes to simply using normal child scenes. And, in the end, what we would need to bridge the gap would be to have a "scene paint mode" that would instantiate scenes in the tile grid, but as children of the TileMapLayer instead. That being said, eventually, I wish I could separate better the responsibilities of the TileMap node (like using a child node for handling the physics, something like that). So maybe we'll find a way to implement this in a simpler way. But for now, the internals are so complex and sensitive to changes that I'd prefer not to change them too much. |
Fair enough! What do we think about:
I think either of these things would save some confusion when trying to figure out the lifetime and ownership of scene tiles. |
I think you are right and the children scenes should indeed be made internal. And improving the docs about scenes tiles there makes a lot of sense. |
|
As long as the order of the children relative to other things isn't critical internal would be useful, and would help with documentation details, emphasizing that you should ensure they don't break if reset etc., avoiding stateful scene tiles unless you handle it yourself by some means, you can do it on exit as the signals are sent in reverse order (just make sure to do it on exiting the tree, not on the canvas as the tiles will be deleted by the time of canvas exit) An idea would be to add a set of methods that could optionally be called on tiles, using |
Fixes #92096
This change stops TileMapLayer from force clearing Scene tiles when the layer exits the tree (but is not freed!).
Tested using the minimal repro project in the issue. Now child scene tiles are preserved when removing/re-adding a TileMap. I also verified that scene tiles are still cleaned up when removed from the TileMap and when the TileMap is freed.